Audit part 2: episode types, proxy credential relay, player state leaks - #25
Conversation
Second pass of the audit, over the areas the first pass never opened: the Android player module, the RSS parser, and the rest of the image/audio proxy. itunes:episodeType was parsed and thrown away parseRSS computed the value, defaulted it to "full", and then left the field out of the struct literal — so every episode ever ingested stored an empty episode_type. The Inbox's "hide specials" filter keys on trailer/bonus and had nothing to key on, degrading to guessing from the title: a properly tagged trailer with a neutral title was never filtered. Atom carries no equivalent tag, so an entry is "full" by the same rule the RSS branch already applied to untagged items. Existing rows heal on the next feed refresh, which already writes episode_type on update. The proxies relayed credentials embedded in a URL Both proxy endpoints take a URL straight out of a publisher's feed. The SSRF-safe dialer resolves and vets every address, on the initial request and on each redirect, but it never looks at the userinfo component — so `https://user:secret@host/` in an artwork or enclosure tag was presented upstream by the instance itself. Only that gap is closed here: repeating the dialer's address policy in the handler would also reject the loopback targets the transport is deliberately allowed to reach. A sleep timer outlived the episode it was set on "Stop at the end of this chapter" is a media position, and only the natural end of an episode cleared it. Skipping to the next track carried the target across and paused the new episode at a position that meant nothing there. The service and the UI state both reset it on an episode change now. The per-podcast outro setting was cached until the podcast changed, so a change made while that show was playing never took effect; it is re-read on the same transition. Verified on an emulator: playback runs, position (34.4 s) and the listening session (34.5 s wall clock, matching audio time at 1.0x) both persist, no crashes. Go and both Android unit-test tasks pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Third pass of the audit, over the remaining server handlers and the Android data layer. An instance could be left with no administrator. SuspendUser counted the active admins outside its write transaction, so two administrators suspending each other at the same time both saw two, both passed the guard, and the result was an instance nobody could administer — recoverable only by editing the database by hand. The count moves inside the transaction, which is BEGIN IMMEDIATE, so the second request waits and sees the first one's effect. Two list endpoints ignored rows.Err(). A read that stops early leaves a short slice that looks like a complete answer: - ListSessions is what a listener uses to spot a session they do not recognise, and the one row missing from a truncated read could be exactly that one. - ExportAccount is offered as the listener's complete copy of their own data. A file that is quietly missing rows is worse than a failed request. Both now fail the request instead. Also corrects the AutoDownloadWorker scheduling comment, which claimed the job runs on an unmetered network while the constraint says CONNECTED. The behaviour is right — that pass only reads feed metadata, and "download over Wi-Fi only" becomes an UNMETERED constraint in DownloadRepository.enqueue, on the transfer that actually spends the listener's data — but the comment described a guarantee the code does not make. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Third pass added to this branchContinued through the remaining server handlers ( An instance could be left with no administrator. Two list endpoints ignored
Both fail the request now. Corrected a misleading comment in Checked and found sound
One product question, not a bug
Still unauditedThe web route components (~7,800 lines across settings/library/search/account/inbox), |
1 participant
Continuation of the audit into the three areas the first pass never opened: the Android player module (2.8k lines),
rss/parser.go, and the remainder ofproxy.go.itunes:episodeTypewas parsed and thrown awayparseRSScomputed the value, defaulted it to"full"— and then left the field out of the struct literal. So every episode ever ingested stored an emptyepisode_type.The Android Inbox's "hide specials" filter keys on it:
With the left half permanently false, the feature degraded to guessing from the title — a correctly tagged
<itunes:episodeType>trailer</itunes:episodeType>with a neutral title was never filtered. Atom carries no equivalent tag, so an entry is"full"by the same rule the RSS branch already applied to untagged items.Existing rows heal on the next feed refresh; the worker's UPDATE already writes
episode_type.The proxies relayed credentials embedded in a URL
Both proxy endpoints take a URL straight out of a publisher's feed — their own comment calls it "fully attacker-controlled". The SSRF-safe dialer resolves and vets every address, on the initial request and again on each redirect, but it never looks at the userinfo component. So
https://user:secret@host/in an artwork or enclosure tag was presented upstream by the instance itself.Only that gap is closed. My first attempt applied the full
rss.ValidateURLand broke four existing tests: it duplicates the dialer's address policy, which also rejects the loopback targets the test transport is deliberately allowed to reach. The check is now narrowed to what the dialer genuinely cannot see.A sleep timer outlived the episode it was set on
"Stop at the end of this chapter" is a media position, and only the natural end of an episode cleared it. Skipping to the next track carried the target across and paused the new episode at a position that meant nothing there. Both the service and the UI state reset it on an episode change now.
Same transition: the per-podcast skip-outro value was cached until the podcast changed, so a change made while that show was playing never took effect.
Verification
go vet+ full suite green, with regression tests for both parser branches, the credential rejection, and the loopback target that must keep working.testDebugUnitTest+testReleaseUnitTestgreen,:app:assembleDebugbuilds.MediaSession state=PLAYING), and after pausing, the position (34,378 ms) and the listening session (34,494 ms wall clock, matching audio time at 1.0×) are both persisted. No crashes.Coverage, honestly
This pass covered roughly 4,000 more lines. Still unaudited: most web route components,
prefs.svelte.ts,idb/db.tsin full, and server-sideopml.go,admin.go,global_stats.go,account.go,itunes/client.go,db/sqlite.go.No version bump — nothing here is urgent enough to re-cut a release on its own.
🤖 Generated with Claude Code